Skip to content

refactor: extract check command wrapper#152

Closed
ndycode wants to merge 2 commits intorefactor/pr1-status-and-featuresfrom
refactor/pr1-check-command
Closed

refactor: extract check command wrapper#152
ndycode wants to merge 2 commits intorefactor/pr1-status-and-featuresfrom
refactor/pr1-check-command

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 20, 2026

Summary

  • extract the codex auth check entry point into a dedicated command module
  • add focused unit coverage for the command wrapper while preserving the existing health-check behavior

What Changed

  • added lib/codex-manager/commands/check.ts as a dedicated check command wrapper
  • updated lib/codex-manager.ts so the check dispatch delegates to the new command module
  • added test/codex-manager-check-command.test.ts to verify the command always invokes runHealthCheck({ liveProbe: true })

Validation

  • npm run lint
  • npm run typecheck
  • npm run test -- test/codex-manager-check-command.test.ts test/codex-manager-status-command.test.ts test/codex-manager-switch-command.test.ts test/codex-manager-cli.test.ts
  • npm run build

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert c37beba to restore the inline check dispatch

Additional Notes

  • this PR stays stacked on #151 so the command split remains easy to review in one-worktree slices

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

extracts the codex auth check dispatch into lib/codex-manager/commands/check.ts using the same dependency-injection pattern established by the sibling status and switch command modules already landed in #151.

  • runCheckCommand is an 8-line wrapper that injects runHealthCheck as a dep — clean, testable, no behavior change
  • codex-manager.ts dispatch is now a single-line delegation: return runCheckCommand({ runHealthCheck })
  • unit tests cover the success path (returns 0) and the rejection path (propagates error), consistent with sibling test files
  • lib/AGENTS.md is not updated to document the commands/ subdirectory; ai agents using the knowledge base will miss check.ts, status.ts, and switch.ts — worth a follow-up update
  • no windows filesystem, token safety, or concurrency concerns in this change — runHealthCheck is a single sequential call with no shared mutable state

Confidence Score: 5/5

  • safe to merge — pure refactor with no behavior change and existing integration coverage in codex-manager-cli.test.ts
  • the dispatch change is semantically identical to the original inline call, the new module follows established patterns, and the integration tests in codex-manager-cli.test.ts already exercise the check command end-to-end. only gap is a documentation update to AGENTS.md and a minor test contract assertion.
  • lib/AGENTS.md — needs updating to document the new commands/ subdirectory

Important Files Changed

Filename Overview
lib/codex-manager/commands/check.ts new 8-line command wrapper — clean DI pattern, consistent with status.ts/switch.ts siblings, no token or filesystem risk
lib/codex-manager.ts single-line dispatch change: await runHealthCheck(...) replaced by return runCheckCommand({ runHealthCheck }) — semantically identical, no behavior delta
test/codex-manager-check-command.test.ts covers success and rejection paths; missing an explicit assertion that any-resolve always returns 0, locking the void-return contract

Sequence Diagram

sequenceDiagram
    participant CLI as runCodexMultiAuthCli
    participant CMD as runCheckCommand
    participant HC as runHealthCheck

    CLI->>CMD: runCheckCommand({ runHealthCheck })
    CMD->>HC: runHealthCheck({ liveProbe: true })
    HC-->>CMD: resolves (void)
    CMD-->>CLI: return 0
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/AGENTS.md
Line: 33

Comment:
**`commands/` subdirectory not documented in knowledge base**

`lib/AGENTS.md` still shows `codex-manager/` as containing only `settings-hub.ts`. this PR (and its base #151) add a `commands/` subdirectory (`check.ts`, `status.ts`, `switch.ts`). any ai agent referencing this guide will miss the new modules entirely.

```suggestion
│   ├── codex-manager/    # settings-hub extraction (2100 lines)
```
should be updated to:
```
│   ├── codex-manager/    # settings-hub + extracted command modules
│   │   ├── commands/     # check, status, switch command wrappers
│   │   └── settings-hub.ts  # extracted interactive settings (2100 lines)
```
similarly, the `WHERE TO LOOK` and `STRUCTURE` sections in `lib/AGENTS.md` need a row for the `commands/` subdir.

**Context Used:** lib/AGENTS.md ([source](https://app.greptile.com/review/custom-context?memory=ffbed5b7-f299-4c32-b3f9-f7c095ca8632))

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/codex-manager-check-command.test.ts
Line: 1-32

Comment:
**missing non-throw failure path test**

the two tests cover `return 0` and `rejects`. there is no test covering the case where `runHealthCheck` resolves successfully but an in-process signal (e.g. a console-printed error summary) means the check conceptually "failed" with exit 0 — which is the current behavior. this is intentional since `runHealthCheck` returns `void`, but it means callers cannot distinguish a clean "all accounts healthy" from a "some accounts degraded but no throw". worth asserting explicitly that the command unconditionally returns `0` on any resolve, to lock the contract and document the limitation.

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "test(check): cover w..."

Context used:

  • Context used - lib/AGENTS.md (source)

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)
  • skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e3b29e8d-a4b7-46c7-a698-5ef75ff7757d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

the pull request extracts the auth check command logic into a dedicated command wrapper module. the main handler in lib/codex-manager.ts delegates to runCheckCommand instead of directly executing runHealthCheck. this establishes a command pattern structure for future extensibility.

Changes

Cohort / File(s) Summary
Main handler refactor
lib/codex-manager.ts
replaced direct runHealthCheck invocation with delegation to new runCheckCommand wrapper; added import statement.
New command module
lib/codex-manager/commands/check.ts
introduced CheckCommandDeps interface with runHealthCheck dependency and runCheckCommand function that returns exit code 0 after invoking the health check with liveProbe: true.
Command unit test
test/codex-manager-check-command.test.ts
added test that verifies runCheckCommand returns exit code 0 and invokes runHealthCheck with correct arguments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Key observations

  • lib/codex-manager.ts:line ? now delegates error handling to the promise chain; confirm that rejection propagates correctly to the cli caller and doesn't mask exit codes.
  • the test at test/codex-manager-check-command.test.ts:line ? only covers the happy path. missing regression test for error scenarios—what happens if runHealthCheck rejects? does the exit code bubble up correctly?
  • runHealthCheck behavior may differ on windows (e.g., signal handling, network probing). flag if this needs platform-specific testing.
  • no concurrent invocation guards visible; if multiple auth check calls overlap, check for shared state in runHealthCheck.
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed the title follows conventional commits format with type refactor, scope check command wrapper, summary is lowercase imperative and 39 characters—well under the 72-char limit.
Description check ✅ Passed the pr description is comprehensive and follows the template structure with all required sections completed: summary, what changed, validation checklist (mostly checked), risk/rollback plan, and additional notes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/pr1-check-command
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr1-check-command

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot previously requested changes Mar 20, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/codex-manager-check-command.test.ts`:
- Around line 8-17: Add a failing-path unit test for runCheckCommand that
asserts it rejects when the injected runHealthCheck throws: create a new test in
the same file that supplies deps: CheckCommandDeps with runHealthCheck:
vi.fn(async () => { throw new Error("boom") }) (or vi.fn(() =>
Promise.reject(new Error("boom")))), call await
expect(runCheckCommand(deps)).rejects.toThrow("boom"), and also assert
runHealthCheck was called with { liveProbe: true }; reference the
runCheckCommand function and the runHealthCheck mock when adding the test.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8174b345-01b9-4479-b192-01440d5cfca1

📥 Commits

Reviewing files that changed from the base of the PR and between 0abd05a and c37beba.

📒 Files selected for processing (3)
  • lib/codex-manager.ts
  • lib/codex-manager/commands/check.ts
  • test/codex-manager-check-command.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

  • lib/codex-manager/commands/check.ts
  • lib/codex-manager.ts
test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

  • test/codex-manager-check-command.test.ts
🔇 Additional comments (2)
lib/codex-manager.ts (1)

39-39: good extraction with behavior parity.

this keeps dispatch clean in lib/codex-manager.ts:5550-5552 while preserving the same auth check behavior through the wrapper import at lib/codex-manager.ts:39. coverage is present in test/codex-manager-check-command.test.ts:8-17. no new windows filesystem or concurrency risk is introduced in this segment.

as per coding guidelines, lib/**: "verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."

Also applies to: 5550-5552

lib/codex-manager/commands/check.ts (1)

1-8: wrapper implementation is clean and deterministic.

the command wrapper in lib/codex-manager/commands/check.ts:1-8 correctly delegates to runHealthCheck({ liveProbe: true }) and returns exit code 0. coverage exists in test/codex-manager-check-command.test.ts:8-17. no windows filesystem or concurrency risk is added here.

as per coding guidelines, lib/**: "verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."

@ndycode ndycode added the passed label Mar 20, 2026
@ndycode ndycode dismissed coderabbitai[bot]’s stale review March 22, 2026 15:43

All review threads are resolved and follow-up commits addressed this stale automated change request.

@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant